fix: preserve SecurityTool schema types in generated docs - #201
Conversation
Signed-off-by: Mohammed Messaoudene <248361935+mohammedmessaoudene-cmd@users.noreply.github.com>
jmeridth
left a comment
jmeridth
left a comment
There was a problem hiding this comment.
🤖 (summary)
Really solid first contribution — thank you. This was reviewed with a multi-model workflow (4 independent reviewers) plus local verification, and the core logic held up against everything we threw at it: adversarial union expressions all fail closed to plain string instead of misprojecting, the Constraint != token.OPTION change exactly reproduces the old required-field semantics under CUE v0.17 (and is needed for the module to compile at all against the pinned dependency), the CommonMark code-span escaping is correct per spec, and make gendocs regenerates deterministically. The regression tests are genuinely strong, especially the fail-closed table.
Three low-severity, non-blocking notes as line comments. Two follow-up ideas beyond this PR's scope, no action needed here: (1) CI could catch future doc drift with a make gendocs && git diff --exit-code step — the gap that allowed #174 in the first place; (2) enum values on named definitions (e.g. a future #Status: "active" | ... referenced via $ref) reach openapi.yaml but aren't yet rendered by openapi2md — same class of silent doc loss this PR fixes, worth a follow-up issue.
| var buf strings.Builder | ||
| fieldLine, description := formatFieldInline(fieldName, fieldSchema, spec, "", isRequired, schemaToFile) | ||
| buf.WriteString(fieldLine + "\n\n") | ||
| if len(fieldSchema.Enum) > 0 { |
There was a problem hiding this comment.
🤖 (low, non-blocking) This renders enums only when they live directly on the field. If a future schema refactor names the union (#Status: "active" | ...) and references it, the converter still emits the enum into openapi.yaml, but both the $ref field line and generateAliasBlock drop it, so the allowed values silently vanish from the docs again. Totally fine to leave out of this PR given your stated scope — mentioning it so we can track it in a follow-up issue.
| got, ok := convertExprToSchema(expr, &OpenAPISpec{}, "").(*SchemaInfo) | ||
| if !ok { | ||
| t.Fatalf("projection type = %T; want *SchemaInfo", got) | ||
| } |
There was a problem hiding this comment.
🤖 (low, non-blocking) Tiny diagnostic nit: in the comma-ok form, got is statically *SchemaInfo, so if this ever failed, %T would print *main.SchemaInfo — the message could never show the actual offending type. Asserting on the interface value first keeps the diagnostic honest:
| got, ok := convertExprToSchema(expr, &OpenAPISpec{}, "").(*SchemaInfo) | |
| if !ok { | |
| t.Fatalf("projection type = %T; want *SchemaInfo", got) | |
| } | |
| result := convertExprToSchema(expr, &OpenAPISpec{}, "") | |
| got, ok := result.(*SchemaInfo) | |
| if !ok { | |
| t.Fatalf("projection type = %T; want *SchemaInfo", result) | |
| } |
| ) | ||
|
|
||
| func TestMarkdownPreservesArrayAndEnumSemantics(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
🤖 (low, non-blocking) Heads-up rather than a change request: convertOpenAPIToMarkdown mutates the package-level linkedHeaders map and never resets it, and this test is the first in-process caller. With one test that's harmless, but the next parallel test that calls this function will race on the map under -race and can pick up footer link definitions from another test's spec. Not something you introduced — just worth knowing if you (or we) add more tests here later.
2 participants
Summary
array[string]in generated OpenAPIContext
The generated Markdown currently documents
SecurityTool.rulesetsasstringeven though the CUE schema defines a string list, and it omits the six allowed values ofSecurityTool.type.This change leaves
spec/schema.cueunchanged. The additionalRepository.statusallowed-values line is a generated consequence of applying the same literal-enum projection to an existing source definition.The converter also replaces the unavailable
ast.Field.Optionalcheck withField.Constraint != token.OPTION, matching the CUE Go v0.17.1 dependency already pinned by the repository.Validation
go test -count=50 ./...in both Go modulesgo test -race ./...in both Go modulesgo vet ./...in both Go modulesScope
This fixes the CUE patterns exercised by the current repository schema. It does not claim complete type inference for every valid CUE expression.
Fixes #174